-
Notifications
You must be signed in to change notification settings - Fork 38.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Local Storage Capacity Isolation API #44785
Conversation
Are you sure you wanted to refer #306? Someone from @kubernetes/sig-storage-api-reviews should be able to review this better. |
pkg/api/types.go
Outdated
@@ -580,6 +580,11 @@ type EmptyDirVolumeSource struct { | |||
// The default is "" which means to use the node's default medium. | |||
// +optional | |||
Medium StorageMedium | |||
// Total amount of local storage required for this directory. | |||
// The default is nil which means that the directory can use all available local storage on the node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this is applicable to "memory" medium as well. Clarify that in the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its not always all available local storage on the node. maybe just say its undefined if omitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
pkg/api/types.go
Outdated
@@ -2944,6 +2949,10 @@ const ( | |||
ResourceMemory ResourceName = "memory" | |||
// Volume size, in bytes (e,g. 5Gi = 5GiB = 5 * 1024 * 1024 * 1024) | |||
ResourceStorage ResourceName = "storage" | |||
// Local Storage for overlay filesystem, in bytes. (500Gi = 500GiB = 500 * 1024 * 1024 * 1024) | |||
ResourceStorageOverlay ResourceName = "storage.kubernetes.io/overlay" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify that the resource names are alpha and that they can change across releases. This is important because a new concept of ResourceClasses
is being discussed where not having a domain name in the user resource name request will be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the domain part, it might be necessary to clarify 'overlay' to make sure people won't confused about the "overlay filesystem driver" and general overlay mechanism used in container, which I think we mean the later here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vishh -- why not just name it alpha like gpu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess it doesnt matter if its behind the feature gate really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will require renaming configs post alpha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -383,7 +383,12 @@ func validateVolumeSource(source *api.VolumeSource, fldPath *field.Path) field.E | |||
allErrs := field.ErrorList{} | |||
if source.EmptyDir != nil { | |||
numVolumes++ | |||
// EmptyDirs have nothing to validate | |||
if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) { | |||
unsetSizeLimit := resource.Quantity{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about validating Pod Spec resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add validation for ResourceRequirements which will be called by validateContainer
pkg/api/validation/validation.go
Outdated
if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) { | ||
unsetSizeLimit := resource.Quantity{} | ||
if unsetSizeLimit.Cmp(source.EmptyDir.SizeLimit) != 0 { | ||
allErrs = append(allErrs, field.Forbidden(fldPath.Child("emptyDir").Child("sizeLimit"), "LocalStorageCapacityIsolation are disabled by feature-gate")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is disabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
/approve |
fc90e24
to
3a0d746
Compare
3a0d746
to
a34af13
Compare
2218864
to
9a0a8a7
Compare
Rebased. @thockin @vishh @derekwaynecarr PTAL |
pkg/api/types.go
Outdated
// The default is nil which means that the limit is undefined. | ||
// More info: http://kubernetes.io/docs/user-guide/volumes#emptydir | ||
// +optional | ||
SizeLimit resource.Quantity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to understand how this field will work with memory backed volumes.
For example, if a pod has a memory request for 500Mi of memory, and has a memory backed volume of 1Gi, the total memory request is still 500Mi, correct? Users are responsible for ensuring that memory backed volumes are sized as part of their pod requests similar to how /dev/shm works? In addition, a BestEffort pod can have a memory backed volume that has a size limit without changing the pod qos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the total memory request is still 500Mi. When specifying the size limit for emptydir, it does not change pod qos
Assuming the size limit for memory backed volumes does not impact QoS or how the pod cgroups are managed, this is LGTM. The scheduler still schedules based on resource requirements only. |
/lgtm |
pkg/api/validation/validation.go
Outdated
if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) { | ||
unsetSizeLimit := resource.Quantity{} | ||
if unsetSizeLimit.Cmp(source.EmptyDir.SizeLimit) != 0 { | ||
allErrs = append(allErrs, field.Forbidden(fldPath.Child("emptyDir").Child("sizeLimit"), "LocalStorageCapacityIsolation is disabled by feature-gate")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Make the error user friendly - SizeLimit field disabled by feature-gate for EmptyDir volumes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nit, otherwise LGTM
I removed the LGTM label to fix the pending nit and add a note on how EmptyDir SizeLimits apply to memory backed volumes as discussed offline. |
9a0a8a7
to
12059d3
Compare
@vishh updated the comment, PTAL |
This PR adds the new APIs to support storage capacity isolation as described in the proposal kubernetes/community#306 1. Add SizeLimit for emptyDir volume 2. Add scratch and overlay storage type used by container level or node level
12059d3
to
695f7be
Compare
updated the comments, add /lgtm back |
@jingxu97: The following test(s) failed:
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/lgtm |
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, jingxu97, thockin, vishh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
bump priority to make sure API change for this feature gets in |
Automatic merge from submit-queue |
This PR adds the new APIs to support storage capacity isolation as
described in the proposal https://github.com/kubernetes/community/pull/306
node level
Release note: